-
Notifications
You must be signed in to change notification settings - Fork 240
fix bug spatial filter #4175 #4286
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
src/spikeinterface/preprocessing/tests/test_highpass_spatial_filter.py
Outdated
Show resolved
Hide resolved
alejoe91
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @oliche!!!
Just a few minor comments
src/spikeinterface/preprocessing/tests/test_highpass_spatial_filter.py
Outdated
Show resolved
Hide resolved
| si_recording = se.read_spikeglx(local_path, stream_id="imec0.ap") | ||
| si_recording = spre.astype(si_recording, "float") | ||
| recording_ps = si.phase_shift(si_recording) | ||
| recording_hp = si.highpass_filter(recording_ps, freq_min=300, filter_order=3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| recording_hp = si.highpass_filter(recording_ps, freq_min=300, filter_order=3) | |
| recording_hp = spre.highpass_filter(recording_ps, freq_min=300, filter_order=3) |
src/spikeinterface/preprocessing/tests/test_highpass_spatial_filter.py
Outdated
Show resolved
Hide resolved
|
|
||
|
|
||
| def agc(traces, window, epsilon=1e-8): | ||
| def agc(traces, window, epsilon=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a problem here because the espilon will change for every chunk no ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would vote to force espilon here because it is an helper function and compute it the init of the class.
no ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but this is a second order issue, we just need to stabilize the inverse of the gain.
To mitigate, I have exposed the epsilon as a parameter in the interface. It would be useful to have a BaseRecording.estimate_rms() method to fix a single epsilon for the full processing, but this is beyond the scope of this PR.
If everyone is on-board for this, I can create an issue and summarize this discussion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me. We hav an get_noise_levels(recording, method="rms") that we could run in the init if rms levels are not estimated yet. But let's discuss specifics on the new issue
Fixed the main bug: all of the processing has to be done in float, even if only to reconvert to int at the end of the module.
I have also improved a few things:
neurodspmodule doesn't exist anymore